ATM: Implement the current endpoint filters as EndpointCharacteristics #11281

tiferet · 2022-11-16T01:12:16Z

📢 This PR is a bit bigger, so commit-by-commit review might be easier. I've tried to make the commit comments as informative as possible.

Main changes

Implement both the standard and the type-specific endpoint filters as EndpointCharacteristics.
Move the definitions of isEffectiveSink and getAReasonSinkExcluded to the base class, as they can now be implemented generically for all sink types.
Changing the definition of getAReasonSinkExcluded is how we'd adjust which endpoints we score at inference time. For now I've implemented it to replicate the logic in the old code, so that results remain unaffected. I've tracked possible experiments to improve this selection in https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2126.

A few notes

Note that this PR still sticks to the principle of not breaking any tests, except that I had to disambiguate three filters from three different sink types that all had the same name (fc56c5a), and that required a tiny update to FilteredTruePositives.expected (0fd013f).

Also note that the training data is unaffected because (for now) I've given all EndpointFilterCharacteristics medium confidence, whereas only high-confidence characteristics contribute to training set selection. AIUI, the reason endpoint filters weren't used to select negative training samples in the old code was precisely this: their accuracy is high enough that we don't want to waste inference time scoring these endpoints, but not high enough that we can reliably use them as negative training samples. It's worth having someone with the needed expertise (Stephan? 😉) go through them eventually to consider whether any should be promoted to high confidence. I tracked this possible experiment in https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2126.

Timing checks

✅ KPI timing experiment: https://github.com/github/codeql-dca-main/issues/8634
☑️ The local runtime of endpoint_large_scale/ExtractEndpointDataTraining remains like it was after the last PR: About 5s.

Closes https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2100

Probably also closes https://github.com/github/ml-ql-adaptive-threat-modeling/issues/2101?

…the last PR

…ntFilterCharacteristic`s

...experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll

Also disambiguate three filters from three different sink types that all have the same name, "not a direct argument to a likely external library call or a heuristic sink".

...l/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

…` to the base class. They can now be implemented generically for all sink types.

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

This is needed because we changed the names of three endpoint filters that were all called "not a direct argument to a likely external library call or a heuristic sink" in order to disambiguate them (fc56c5a).

…tFeatures` overrides it.

adityasharad

Nice. I can see the existing filtering logic translated into the new object-oriented hierarchy, and that you've taken care to preserve that logic.

My suggestions are mostly about the clarity and performance of the QL code, rather than the semantics of the filters. Most are not blocking but I hope will be useful.

...l/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

...l/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/StandardEndpointFilters.qll

...l/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

kaeluka

I love love love how this is remolding logic that used to be really hard to manage into something more maintainable. Things are really starting to fall into place! Thank you :)

The one complaint I have about this is how the StandardEndpointFilterCharacteristic class is being made magic by its usage in other modules. The bright side is that there's very easy fixes that I have outlined in my comments. Addressing these should be doable in 20-30min.

...l/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

...experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll

...pt/ql/experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.ql

...experimental/adaptivethreatmodeling/test/endpoint_large_scale/FilteredTruePositives.expected

...pt/ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/XssATM.qll

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

...l/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/EndpointCharacteristics.qll

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

Make `SyntacticHeuristics` an explicit import

kaeluka · 2022-11-22T13:48:36Z

I'm still working on this review, trying to figure out what the real problem is here and coming up with a specific solution. The PR's interdependencies are quite complex, which is why this has taken a little.

kaeluka · 2022-11-22T15:31:49Z

I think I'm through with the review.

🤡 The PR sadly reproduces some of the confusing definitions we currently have in main. How the classes StandardEndpointFilterCharacteristic and EndpointFilterCharacteristic are used is quite confusing. This isn't a blocker though, as these classes will probably change LOTS during future experimentation. This is why I think we can safely ignore this paper cut for now and should focus on getting things done.

I'm a bit sick and will call it a day now.

✅ I consider the review of this PR done and approved, assuming you are making the StandardEndpointFilterCharacteristic private (for example, by adding the commit I linked above).
✅ I reviewed the PR that's removing a lot of files (😍) and all it'd need to be merged is a rebase.
🕥 Tomorrow, I'll work on reviewing the third PR (#11323) but to manage your expectations: that one sounds complex and might take a while to fully grok.

tiferet · 2022-11-22T16:46:56Z

(Minor request: Do you mind using words rather than emoji? I don't remember off the top of my head what each emoji was meant to signify, and I had to spend some time searching for the slack thread to look up what 🤡 means)

tiferet · 2022-11-22T18:08:18Z

...experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll

+  // Don't surface endpoint filters as characteristics, because they were previously not surfaced.
+  // TODO: Experiment with surfacing these to the modeling code by removing the following line (and then make
+  // EndpointFilterCharacteristic private).
+  not characteristic instanceof EndpointFilterCharacteristic and


OK, I see why your commit didn't fail endpoint_large_scale/ExtractEndpointDataTraining.qlref: It's because it didn't remove this line. If we were to raise the confidence of the endpoint filters without having this line, the training set would include all non-effective-sinks, which would be bad, because many of those are in fact low-confidence characteristics.

Do you see a point in making StandardEndpointFilterCharacteristic private without removing this line or making EndpointFilterCharacteristic private?

IMO, I think we need to be consistent: Either we use subclasses of EndpointCharacteristics to replicate the existing data for now, or we try to improve the logic at the same time that we improve the code, and each PR requires end-to-end testing. I definitely prefer the former. The nice thing is that, once we're ready to progress to the second phase, even without my TODO comments and issue tracking, we can easily look at all EndpointCharacteristics that aren't private and find the places they're used in the code, and that will tell us where we want to make improvements. Ultimately all EndpointCharacteristics should be private, with subclasses used only for two things: (1) Abstract classes are used to apply the same set of implications to many different subclasses without needing to overwrite getImplications() over and over. (2) Non-abstract classes are used to define the labels for type balancing through getEndpoints().

tiferet · 2022-11-22T18:35:03Z

✅ I consider the review of this PR done and approved, assuming you are making the StandardEndpointFilterCharacteristic private (for example, by adding the commit I linked above).

Unfortunately I don't think I can do that 😞. Or rather, I can do it by introducing a new confidence level, but that would further hide ugliness in a way that will make it harder to fix later. I know this back-and-forth is frustrating, and I'm sure a quick sync would be an easier way to reach resolution, but we can do that when you're feeling better -- please don't push yourself!

✅ I reviewed the PR that's removing a lot of files (😍) and all it'd need to be merged is a rebase.

Thank you! ❤️

🕥 Tomorrow, I'll work on reviewing the third PR (#11323) but to manage your expectations: that one sounds complex and might take a while to fully grok.

No rush! There's a chance that one is trickier than I realized. Luckily, if we do need big changes on that one, there's only one PR following it in the chain, so it won't require big changes to a long chain of subsequent PRs. Anyway, Thursday and Friday are bank holidays in the US, so unless that PR is unexpectedly ready for approval and merging tomorrow, it won't get merged before Monday. More importantly, get lots of rest and get healthy!

kaeluka · 2022-11-23T11:49:16Z

More importantly, get lots of rest and get healthy!

Thank you! I agree, there's no point rushing this. I see several ways forward, we only need to find one that works for both of us. And enjoy your time off!

Select endpoints to score at inference time base purely on their confidence level, and not on whether they fit the historical definition of endpoint filters.

Endpoint filters added commits

tiferet · 2022-11-28T19:23:00Z

@kaeluka I merged this change we agreed on into this PR, and updated the predicate's documentation accordingly. I think this addresses all your concerns, unless I lost track of something else in the long discussion 😅 Sending this back to you for final(?) review 🏓

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll

I've been working with Brits for too long :)

kaeluka

🥳 👍

Delete the file ExtractEndpointData.expected which was leftover in …

cb632b3

…the last PR

github-actions bot added the ATM label Nov 16, 2022

tiferet added 2 commits November 15, 2022 17:20

Implement the standard endpoint filters as EndpointCharacteristics

cf4e37a

Implement the standard getAReasonSinkExcluded using `StandardEndpoi…

fedb98d

…ntFilterCharacteristic`s

github-advanced-security bot found potential problems Nov 16, 2022

View reviewed changes

...experimental/adaptivethreatmodeling/modelbuilding/extraction/ExtractEndpointDataTraining.qll Fixed Show fixed Hide fixed

tiferet added 2 commits November 15, 2022 17:29

Delete some code that's no longer in use

2ecdfd1

Fix CodeQL warning

13cb0ab

owen-mc changed the title ~~Implement the current endpoint filters as EndpointCharacteristics~~ ATM: Implement the current endpoint filters as EndpointCharacteristics Nov 16, 2022

Implement the type-specific endpoint filters as EndpointCharacteristics.

fc56c5a

Also disambiguate three filters from three different sink types that all have the same name, "not a direct argument to a likely external library call or a heuristic sink".

github-advanced-security bot found potential problems Nov 16, 2022

View reviewed changes

Move the definitions of isEffectiveSink and `getAReasonSinkExcluded…

eab270e

…` to the base class. They can now be implemented generically for all sink types.

github-advanced-security bot found potential problems Nov 16, 2022

View reviewed changes

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll Fixed Show fixed Hide fixed

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll Fixed Show fixed Hide fixed

tiferet added 5 commits November 16, 2022 11:54

Update the reason names in FilteredTruePositives.expected.

0fd013f

This is needed because we changed the names of three endpoint filters that were all called "not a direct argument to a likely external library call or a heuristic sink" in order to disambiguate them (fc56c5a).

Be explicit in requiring that each ATM config set its endpoint type.

c2035e8

Fix CodeQL warnings

8fee9cb

isEffectiveSink can't be final because `ExtractMisclassifiedEndpoin…

38c40a7

…tFeatures` overrides it.

Add a comment

ccbf1ca

tiferet marked this pull request as ready for review November 16, 2022 21:25

tiferet requested review from a team and kaeluka and removed request for a team November 16, 2022 21:25

Remove some imports that are no longer used

4a13829

tiferet mentioned this pull request Nov 17, 2022

ATM: Remove redundant code #11321

Merged

adityasharad reviewed Nov 18, 2022

View reviewed changes

Suggestions from code review

8d22fd2

tiferet requested a review from adityasharad November 19, 2022 00:02

kaeluka suggested changes Nov 21, 2022

View reviewed changes

Address comment from code review:

1c9545e

Make `SyntacticHeuristics` an explicit import

tiferet commented Nov 22, 2022

View reviewed changes

tiferet requested a review from kaeluka November 22, 2022 18:09

tiferet added 3 commits November 23, 2022 10:46

Filter endpoints by confidence

03b8e64

Select endpoints to score at inference time base purely on their confidence level, and not on whether they fit the historical definition of endpoint filters.

Update the documentation

963407d

Merge pull request #11462 from github/tiferet/endpoint-filters-sidebar

72c46c6

Endpoint filters added commits

github-advanced-security bot found potential problems Nov 28, 2022

View reviewed changes

...ql/experimental/adaptivethreatmodeling/lib/experimental/adaptivethreatmodeling/ATMConfig.qll Fixed Show fixed Hide fixed

Fix British spelling that code scanning didn't like.

7b0269c

I've been working with Brits for too long :)

kaeluka approved these changes Nov 29, 2022

View reviewed changes

tiferet merged commit f375b0c into main Nov 29, 2022

tiferet deleted the tiferet/endpoint-filters branch November 29, 2022 20:38

ATM: Implement the current endpoint filters as EndpointCharacteristics #11281

ATM: Implement the current endpoint filters as EndpointCharacteristics #11281

Uh oh!

Conversation

tiferet commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Main changes

A few notes

Timing checks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaeluka left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaeluka commented Nov 22, 2022

Uh oh!

kaeluka commented Nov 22, 2022

Uh oh!

tiferet commented Nov 22, 2022

Uh oh!

tiferet Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiferet commented Nov 22, 2022

Uh oh!

kaeluka commented Nov 23, 2022

Uh oh!

tiferet commented Nov 28, 2022

Uh oh!

Uh oh!

kaeluka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tiferet commented Nov 16, 2022 •

edited

Loading

kaeluka left a comment •

edited

Loading

tiferet Nov 22, 2022 •

edited

Loading